backport: bitcoin/bitcoin#24304: [kernel 0/n] Introduce dash-chainstate#7234
backport: bitcoin/bitcoin#24304: [kernel 0/n] Introduce dash-chainstate#7234knst wants to merge 4 commits into
dash-chainstate#7234Conversation
|
This pull request has conflicts, please rebase. |
|
eb2142d fix: added missing thread annotation for CJWalletManagerImpl (Konstantin Akimov) 40cd08d refactor: rename CCoinJoinBaseManager to CoinJoinQueueManager and make it non-virtual (Konstantin Akimov) 6fae09a refactor: remove inheritance from CCoinJoinBaseManager in src/test/coinjoin_basemanager_tests.cpp (Konstantin Akimov) a0f39f2 refactor: removed CCoinJoinBaseManager from inheritance of CCoinJoinServer and make it a member (Konstantin Akimov) 72229ce refactor: inline class CCoinJoinClientQueueManager to CJWalletManagerImpl (Konstantin Akimov) 3b1c2da refactor: inline the middleman class CoinJoinWalletManager to CJWalletManagerImpl (Konstantin Akimov) e2d8247 refactor: pass directly reference to CCoinJoinClientManager& instead unique_ptr inside ForEachCJClientMan (Konstantin Akimov) 689b4ec refactor: remove passing pointer to unique_ptr to more clear sequence of object initializtion in CJ (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Current coinjoin subsystem have multiple classes that inherits from each other but some of them doesn't provide any real abstraction level yet complicate architecture. CJWalletManagerImpl, CCoinJoinClientManager, and CCoinJoinClientQueueManager are 2 separate pieces of implementation of CJWalletManager while CJWalletManagerImpl is already private and could keep all implementation inside. ## What was done? This PR simplify CJ architecture by removing boiler code and unneeded abstraction levels to make code easier to support, read and improve. The changes for this PR has been spotted and half-done during #7234 but it's not a direct dependency of final solution, so far as it was possible to fully cut CJ from dash-chainstate implementation. - replaces all references to unique_ptr to just a reference or just unique_ptr to more clear sequence of object initialization in CJ and ownership - inline the middleman class CoinJoinWalletManager to CJWalletManagerImpl which is already private and hidden - remove class CCoinJoinClientQueueManager; logic inlined into CJWalletManagerImpl - remove interface CoinJoinQueueNotify - que notification no longer needed as a separate abstraction - replaced inheritance of CCoinJoinServer : public CCoinJoinBaseManager to composition (m_queueman member) - remove inheritance of regression tests from CCoinJoinBaseManager - rename CCoinJoinBaseManager to CoinJoinQueueManager and make it non-virtual ## CoinJoin Class Diagram — Before vs After [by claude] BEFORE CCoinJoinBaseManager CoinJoinQueueNotify (virtual ctor/dtor) (interface) - vecCoinJoinQueue - OnQueueUpdate() [pure] - cs_vecqueue ▲ ▲ │ inherits │ implements ├─────────────────┐ │ CCoinJoinClientQueueManager CCoinJoinClientManager (owned by CJWalletManagerImpl) (map entry per wallet) │ │ inherits │ CCoinJoinServer ──also inherits──▶ CCoinJoinBaseSession ▲ │ inherits CCoinJoinClientSession CJWalletManager (abstract) └── CJWalletManagerImpl ├── unique_ptr<CCoinJoinClientQueueManager> m_basequeueman └── map<wallet* → CCoinJoinClientManager> --- AFTER CoinJoinQueueManager ← renamed (no C prefix), no virtual ctor/dtor - vecCoinJoinQueue no longer a base class at all - cs_vecqueue + TryHasQueueFromMasternode() new TRY_LOCK helpers + TryCheckDuplicate() + TryAddQueue() CCoinJoinServer ──inherits──▶ CCoinJoinBaseSession - m_queueman: CoinJoinQueueManager ← OWNED by value (composition) ▲ │ inherits CCoinJoinClientSession CJWalletManager (abstract) └── CJWalletManagerImpl ├── unique_ptr<CoinJoinQueueManager> m_queueman ← nullptr if !relay_txes └── map<wallet* → CCoinJoinClientManager> │ │ CCoinJoinClientManager │ - m_queueman: CoinJoinQueueManager* ← non-owning ptr │ (points into CJWalletManagerImpl::m_queueman) ## How Has This Been Tested? Run unit & functional tests. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK eb2142d Tree-SHA512: 03a648885d86c6e251296b3315ddf2ffb0ab3839526cf10a52bb36f53dc7ff1888bc45f69bbdcddd8a32bd940955cf6e8487e84062d0dde9daa32317721eeb5a
…andleNewRecoveredSig to variant eeb84cb refactor: replace usages of MessageProcessingResult in HandleNewRecoveredSig to variant (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented The public interface `HandleNewRecoveredSig` which is widely used on develop returns `MessageProcessingResult` Using MessageProcessingResult is incorrect using here; because `HandleNewRecoveredSig` may return only CInv or CTransactionRef. Using MessageProcessingResult here spreads including of heavy `msg_result.h` all over codebase; it makes dependency of llmq/ code on network code. This PR is a direct dependency for kernel / dash-chainstate project, see #7234 ## What was done? Replaced MessageProcessingResult to std::variant<CInv, CTransactionRef, std::monostate>. Potentially, even CTransactionRef is overhead and should be replaced to special case of CInv, but it's out-of-scope of this refactoring. Also, `m_clhandler.ProcessNewChainLock` should be refactored and split to network and non-network part and avoid leaking `MessageProcessingResult` abstraction here too, but I kept it out-of-scope of this refactoring too. ## How Has This Been Tested? Run unit tests & functional tests. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: thepastaclaw: crACK eeb84cb Clean refactoring that replaces the generic `MessageProcessingResult` with a precise `std::variant<std::monostate, CInv, CTransactionRef>` for `HandleNewRecoveredSig`. This addresses the TODO that was already in the code. Verified: - **ChainLock handler**: `ProcessNewChainLock(-1, ...)` can only produce a single `CInv` or empty result (no `MisbehavingError` when `from == -1`, no `m_transactions`), so extracting `m_inventory.front()` is correct. The TODO comment about splitting `ProcessNewChainLock` is a good note for follow-up. - **EHF handler**: Now returns `CTransactionRef` directly instead of pushing `GetHash()` into `m_transactions`. The caller in `net_signing.cpp` calls `PeerRelayTransaction(hash)` which routes through `RelayTransaction` → `_RelayTransaction` — same codepath as the old `PostProcessMessage` loop over `m_transactions`. - **InstantSend + SigShares handlers**: Always returned empty `MessageProcessingResult{}`, now correctly return `std::monostate{}`. - **Dispatch in `net_signing.cpp`**: Clean `std::get_if` dispatch replacing the opaque `PeerPostProcessMessage` call. Each variant arm calls the appropriate relay function. - `PeerPostProcessMessage` remains in `net_processing.h` for other callers — no dangling references. UdjinM6: utACK eeb84cb Tree-SHA512: 6158fc345ad9a70d2b0d4da0b5c3cfef76642e16d5738fab3aebdc771f68f4b3074bb7638864a3574ae66ad58123341dcd5dd83d39d7fb0737881129809b6427
|
This pull request has conflicts, please rebase. |
…uorum participant 863ee6b doc: clarify masternode late InitializeCurrentBlockTip comment (UdjinM6) dc8c6cf fix: replay startup tip into ActiveContext/ObserverContext (UdjinM6) 21fe75f fix: guard GetProTxHash() behind is_masternode check, add CQuorum safety assert (UdjinM6) 27129d9 fix: defer full InitializeCurrentBlockTip broadcast on masternodes (UdjinM6) 55d8025 refactor: safety belt to be sure that CQuorum is initialized properly and no empty qc member (Konstantin Akimov) 8bc5744 fix: review comments - missing forward declaration, protection for no valid members (Konstantin Akimov) e2df648 refactor: drop argument request_limit_exceeded in ProcessContribQGETDATA (Konstantin Akimov) b47de25 refactor: remove net related code ProcessContribQGETDATA and ProcessContribQDATA to NetQuorum (Konstantin Akimov) 1be8863 refactor: move out network code to new NetQuorum from CQuorumManager (Konstantin Akimov) c86e4f6 refactor: it's always either observer_ctx or active_ctx, not both created together (Konstantin Akimov) 6a2f291 refactor: remove duplicated code between QuorumRole's UpdatedBlockTip and InitializeQuorumConnections (Konstantin Akimov) 6c99cb0 fix: parameter 'params' shadows member inherited from type 'CDKGSession' [-Werror,-Wshadow-field] (Konstantin Akimov) 0f1174a refactor: remove multiple friends of CDKGSession (Konstantin Akimov) f4cbf92 refactor: inline QuorumParticipant to ActiveContext (Konstantin Akimov) 5ded711 refactor: rename src/llmq/observer/context to src/llmq/observer (Konstantin Akimov) 93f168a refactor: remove QuorumRoleBase by moving its functionality directly to QuorumRole (Konstantin Akimov) 9bb50db refactor: merge QuorumObserver and ObserverContext classes to the one entity to reduce amount of abstractions (Konstantin Akimov) 7978822 refactor: quorum members: remove QuorumObserverParent; participant is no more child of 'observer' (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented It's a follow-up for #7066 and related PRs This PR is reducing mental concentration when reading quorum-related code by significant simplification amount of levels of abstractions and inheritance classes from each other. This PR is a direct dependency of #7234 (kernel / chainstate project) Performance is not expected to be improved by any noticeable margin. Compilation time is expected to be improved marginally due to fewer files (-2) to compile and less transitive includes over `src/llmq/quorumsman.h`. ## What was done? This changes has been spotted during #7234 and extracted to own PR. - Remove QuorumObserverParent; decouple QuorumParticipant from QuorumObserver inheritance — they become independent implementations of a common QuorumRole interface. - Merge QuorumObserver and ObserverContext into a single class, then remove the transitional QuorumRoleBase by folding it into QuorumRole. - Rename src/llmq/observer/context.{h,cpp} to src/llmq/observer.{h,cpp} — flatten the now-single-file subdirectory. - Inline QuorumParticipant into ActiveContext — it only applies to active masternode mode; src/active/quorums.{h,cpp} deleted. - Remove four friend declarations from CDKGSession (ActiveDKGSession, ActiveDKGSessionHandler, CDKGSessionHandler, CDKGSessionManager); affected members changed from private to protected. - Extract network code from CQuorumManager and ActiveContext into new NetQuorum (src/llmq/net_quorum.{h,cpp}) — handles QGETDATA/QDATA processing, quorum peer connections, data recovery, and periodic cleanup. ### Class hierarchy — before vs. after *by claude Before: QuorumObserverParent (interface, implemented by CQuorumManager) │ QuorumObserver (base class with shared state) ├── ObserverContext (observer mode, also CValidationInterface) └── QuorumParticipant (active mode, in src/active/quorums.h) After: QuorumRole (pure virtual interface in quorumsman.h) ├── ObserverContext (observer mode, in src/llmq/observer.h) └── ActiveContext (active mode, QuorumParticipant inlined) ## How Has This Been Tested? Run unit & functional tests ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK 863ee6b Tree-SHA512: 13a3d8e533bf811fd5d438e0c2f3007b633e990b725767907d5c4b8eb2da32d06f282f3f61319ce78a17e40ca41dac1b3c791039c5672321e1b43a7174712b2d
|
This pull request has conflicts, please rebase. |
0bcb345 refactor: move VerifyChainLock and ChainLockSig between modules to untangle chain-state dependencies (Konstantin Akimov) 3441f04 refactor: move multiple msg_result.h usages headers to implementation (Konstantin Akimov) 69d4a06 feat: better loging of p2p governance objects, separate local and pendings (Konstantin Akimov) f4ee8b4 refactor: drop dependency of CGovernanceManager on net.cpp (Konstantin Akimov) 646c2c8 fix: circular dependency over rpc/evo_util (Konstantin Akimov) 2b2d0a4 refactor: move CDeterministicMN::ToJson to core_write to avoid g_txindex dependency for linking (Konstantin Akimov) 312ca8e refactor: drop pfrom from CGovernanceManager::ProcessVote (Konstantin Akimov) d0414cc refactor: drop dependency of governance/object on core_write (ScriptToAsmStr) (Konstantin Akimov) aa4e692 fix: add guards if CTxMempool is nullptr in chainstate (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented That's some final preparation with various refactors for kernel-0 project, splitted out from #7234 ## What was done? - drop pfrom from CGovernanceManager::ProcessVote - drop dependency of CGovernanceManager on net.cpp - drop dependency of governance/object on core_write (ScriptToAsmStr) - add guards for CTxMempool is nullptr [which could be for chainstate case] - removed multiple includes of msg_result.h over codebase - moved CDeterministicMN::ToJson to core_write to avoid g_txindex - move VerifyChainLock and ChainLockSig between modules to untangle chain-state dependencies NOTE: the circular dependency over `dkgsessionhandler <-> net_processing` is not new, it's pre-existing one but discovered by replacing removing `llmq/dkgsessionhandler.h` from `llmq/dkgsessionmgr.h` and direct adding this include to llmq/observer "llmq/dkgsessionhandler -> net_processing -> llmq/dkgsessionmgr -> llmq/dkgsessionhandler", "llmq/dkgsessionhandler -> net_processing -> llmq/observer -> llmq/dkgsessionhandler", ## How Has This Been Tested? Run unit & functional tests; proof-of-concept of final PR is #7234 Compilation time is not affected by this PR: removing msg_result.h from the header have too small positive impact; replacing chainlock/clsig.h to chainlock/chainlock.h doesn't give any visible change either. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 0bcb345 Tree-SHA512: 944adb2da273ac4f72f4c63a5be19511c1cabcee601ea03594a5bfe8cd65ff82512011be2e644fb12bf16c20a4149fcfa1c641df539a32c80cd511335a25c2e3
4e6f844 refactor: collapse spork signer recovery to one ECDSA op, restore peer info in logs (UdjinM6) 6a4ee52 fix: replace assert to if-return. Less probability to get troubles in the future (Konstantin Akimov) 7460211 refactor: address review feedback on spork ProcessMessage refactor (UdjinM6) 7c899a2 refactor: drop dependency of spork manager on network code (Konstantin Akimov) bd2bff1 refactor: move network implementation of spork to net_processing (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented It helps to break circular dependencies over spork <-> net. It helps to get spork manager ready kernel project. Apparantly, chainstate implementation uses sporks for validation of blockchain but network dependencies could be avoided there. This changes is PR-ready changes from #7234 for spork manager. ## What was done? Dropped dependency of spork manager on network code by moving network code to net_processing. There's no need dedicated NetHandler because imlementation is trivial, short and doesn't have any async worker or threads. ## How Has This Been Tested? Run unit & functional tests. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 4e6f844 Tree-SHA512: 5b06e49c716cf66b66ffe9902fa0f987d0492236b87c45eb285d30def49a2bc66f8ef21b0ae4372465de4598b02f17605c5e127bab79bf2f35c83f0e819b7caf
…otifier is introduced to use it for CChainState
BACKPORT NOTES: - UpdateUncommittedBlockStructures is segwit related; omitted from bitcoin-chainstate.cpp - call chainman.UnloadBlockIndex is removed by bitcoin#22564 - binary name is renamed from bitcoin-chainstate to dash-chainstate. Names of source files and some variables are untouched - added LIBDASHBLS to list of libraries to link - couple functions are reimplemented to avoid balooning dash-chainstate by including extra heavy depenencies - ValueFromAmount (reimplemented, core_write.cpp) - GetPrettyExceptionStr (reimplemented, stacktrackes.cpp) - g_stats_client (redefined) There are several modules that should be removed in the near future from dash-chainstate binary - protocol.cpp move NetMsgType inline helpers out of chainlock/clsig.h, governance/object.h, evo/simplifiedmns.h into their .cpp files - llmq/dkgsession.cpp, llmq/dkgsessionhandler.cpp, llmq/dkgsessionmgr.cpp, llmq/debug.cpp, batchedlogger.cpp CQuorumManager already null-guards m_qdkgsman; drop once ConnectManagers() is never called on the kernel side - llmq/signing.cpp split CSigningManager out of LLMQContext into a post-construction Connect() so the context becomes quorum-only - governance/*.cpp promote the 4 methods used by CMNPaymentsProcessor (IsValid, IsSuperblockTriggered, IsValidSuperblock, GetSuperblockPayments) to a base interface; instantiate a NullGovernanceManager in bitcoin-chainstate.cpp ORIGINAL PR Description: 2c03cec ci: Build bitcoin-chainstate (Carl Dong) 095aa6c build: Add example bitcoin-chainstate executable (Carl Dong) Pull request description: Part of: bitcoin#24303 This PR introduces an example/demo `bitcoin-chainstate` executable using said library which can print out information about a datadir and take in new blocks on stdin. Please read the commit messages for more details. ----- #### You may ask: WTF?! Why is `index/*.cpp`, etc. being linked in? This PR is meant only to capture the state of dependencies in our consensus engine as of right now. There are many things to decouple from consensus, which will be done in subsequent PRs. Listing the files out right now in `bitcoin_chainstate_SOURCES` is purely to give us a clear picture of the task at hand, it is **not** to say that these dependencies _belongs_ there in any way. ### TODO 1. Clean up `bitcoin-chainstate.cpp` It is quite ugly, with a lot of comments I've left for myself, I should clean it up to the best of my abilities (the ugliness of our init/shutdown might be the upper bound on cleanliness here...) ACKs for top commit: ajtowns: ACK 2c03cec ryanofsky: Code review ACK 2c03cec. Just rebase, comments, formatting change since last review MarcoFalke: re-ACK 2c03cec 🏔 Tree-SHA512: 86e7fb5718caa577df8abc8288c754f4a590650d974df9d2f6476c87ed25c70f923c4db651c6963f33498fc7a3a31f6692b9a75cbc996bf4888c5dac2f34a13b
bitcoin-chainstatedash-chainstate
|
✅ Review complete (commit 99d2079) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99d2079bfc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| # Required for obj/build.h to be generated first. | ||
| # More details: https://www.gnu.org/software/automake/manual/html_node/Built-Sources-Example.html | ||
| bitcoin_chainstate-clientversion.$(OBJEXT): obj/build.h |
There was a problem hiding this comment.
Rename build.h dependency to dash_chainstate target
The dependency is attached to bitcoin_chainstate-clientversion.$(OBJEXT), but the program target introduced in this patch is dash-chainstate, so Automake will emit object names under the dash_chainstate-... prefix instead. As written, this prerequisite never matches the actual clientversion.cpp object for dash-chainstate, so in parallel builds clientversion.cpp can be compiled before obj/build.h exists (it includes <obj/build.h> when HAVE_BUILD_INFO is enabled), causing intermittent build failures.
Useful? React with 👍 / 👎.
WalkthroughThis PR introduces the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/llmq/net_quorum.h (1)
10-10: ⚡ Quick winTODO comment for removing
llmq/utils.hinclude.The comment indicates this include should be removed in the future. Verify that moving the quorum connection logic truly eliminates the need for this transitive dependency, or if types/utilities from
llmq/utils.hare still required by the declarations in this header.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llmq/net_quorum.h` at line 10, The TODO notes removing the `#include` <llmq/utils.h> from src/llmq/net_quorum.h; inspect the public declarations in net_quorum.h for any direct uses of types, enums, or functions declared in llmq/utils.h and either (A) remove the include if none are used, (B) replace it with precise includes for the specific headers that declare the referenced symbols, or (C) add forward declarations for llmq types used only by pointer/reference in declarations (so the header no longer depends on llmq/utils.h). Update net_quorum.h so it no longer relies on transitive includes from llmq/utils.h while keeping compile errors fixed (focus on symbols referenced in declarations within net_quorum.h).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bitcoin-chainstate.cpp`:
- Line 87: Wrap the std::filesystem::create_directories(abs_datadir) call with
error handling: catch std::filesystem::filesystem_error (or std::exception)
around that call, log a clear error message including abs_datadir and the
exception text (e.what()), and then propagate failure by returning an error /
setting an appropriate error code or calling process exit so later code doesn't
run on a missing directory; alternatively check existence after the call and
handle the failed creation path accordingly.
---
Nitpick comments:
In `@src/llmq/net_quorum.h`:
- Line 10: The TODO notes removing the `#include` <llmq/utils.h> from
src/llmq/net_quorum.h; inspect the public declarations in net_quorum.h for any
direct uses of types, enums, or functions declared in llmq/utils.h and either
(A) remove the include if none are used, (B) replace it with precise includes
for the specific headers that declare the referenced symbols, or (C) add forward
declarations for llmq types used only by pointer/reference in declarations (so
the header no longer depends on llmq/utils.h). Update net_quorum.h so it no
longer relies on transitive includes from llmq/utils.h while keeping compile
errors fixed (focus on symbols referenced in declarations within net_quorum.h).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b488a754-51c5-47cc-903b-b31a03f5095c
📒 Files selected for processing (14)
.gitignoreci/dash/matrix.shci/test/00_setup_env_native_nowallet_libbitcoinkernel.shconfigure.acsrc/Makefile.amsrc/active/dkgsessionhandler.cppsrc/active/dkgsessionhandler.hsrc/bitcoin-chainstate.cppsrc/llmq/net_quorum.cppsrc/llmq/net_quorum.hsrc/llmq/utils.cppsrc/llmq/utils.hsrc/masternode/sync.cppsrc/masternode/sync.h
💤 Files with no reviewable changes (2)
- src/llmq/utils.h
- src/llmq/utils.cpp
| return 1; | ||
| } | ||
| std::filesystem::path abs_datadir = std::filesystem::absolute(argv[1]); | ||
| std::filesystem::create_directories(abs_datadir); |
There was a problem hiding this comment.
Add error handling for directory creation.
The std::filesystem::create_directories call can fail (e.g., due to permission errors or disk space issues), but there's no check for success. If directory creation fails, subsequent operations will encounter less-clear error messages.
🛡️ Proposed fix to check directory creation
std::filesystem::path abs_datadir = std::filesystem::absolute(argv[1]);
-std::filesystem::create_directories(abs_datadir);
+if (!std::filesystem::exists(abs_datadir) && !std::filesystem::create_directories(abs_datadir)) {
+ std::cerr << "Failed to create directory: " << abs_datadir << std::endl;
+ return 1;
+}
gArgs.ForceSetArg("-datadir", abs_datadir.string());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::filesystem::create_directories(abs_datadir); | |
| std::filesystem::path abs_datadir = std::filesystem::absolute(argv[1]); | |
| if (!std::filesystem::exists(abs_datadir) && !std::filesystem::create_directories(abs_datadir)) { | |
| std::cerr << "Failed to create directory: " << abs_datadir << std::endl; | |
| return 1; | |
| } | |
| gArgs.ForceSetArg("-datadir", abs_datadir.string()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bitcoin-chainstate.cpp` at line 87, Wrap the
std::filesystem::create_directories(abs_datadir) call with error handling: catch
std::filesystem::filesystem_error (or std::exception) around that call, log a
clear error message including abs_datadir and the exception text (e.what()), and
then propagate failure by returning an error / setting an appropriate error code
or calling process exit so later code doesn't run on a missing directory;
alternatively check existence after the call and handle the failed creation path
accordingly.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two real build defects in src/Makefile.am are confirmed: the obj/build.h dependency rule references the upstream target name (bitcoin_chainstate-clientversion) instead of the renamed dash_chainstate-clientversion, and dash_chainstate_LDADD references an undefined $(LIBLEVELDB_SSE42). One additional suggestion about the experimental binary's shutdown order is worth surfacing. Most remaining findings either describe intentional, explicitly-documented adaptations for the experimental kernel binary (NullNodeSyncNotifier-induced behavior, mainnet hardcoding) or are low-value nitpicks.
Reviewed commit: 99d2079
🟡 3 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/Makefile.am`:
- [SUGGESTION] lines 1318-1321: obj/build.h dependency rule uses old upstream target name
Because `dash_chainstate` sets per-target CPPFLAGS/CXXFLAGS (lines 1306-1307), automake compiles its sources to per-target objects named `dash_chainstate-<basename>.$(OBJEXT)` (same convention as `libbitcoin_util_a-clientversion.$(OBJEXT)` on line 482). The rule `bitcoin_chainstate-clientversion.$(OBJEXT): obj/build.h` was copied verbatim from upstream and references a file automake will never produce for this binary, so it never enforces that `obj/build.h` is generated before `clientversion.o` is compiled. The PR's own merge note says the binary was renamed but "Names of source files and some variables are untouched" — this rule is one of the spots that did need to be renamed for the renamed target.
- [SUGGESTION] lines 1309-1316: dash_chainstate_LDADD references undefined variable LIBLEVELDB_SSE42
`LIBLEVELDB_SSE42` is not defined anywhere in the tree. `src/Makefile.leveldb.include` only defines `LIBLEVELDB = $(LIBLEVELDB_INT) $(LIBCRC32C)` and `LIBMEMENV`; a grep for `LIBLEVELDB_SSE42` finds only this LDADD line. Make silently expands an unset variable to empty so this is dead text rather than a build failure today, but it is misleading (implies SSE4.2 acceleration is being linked when it isn't) and would silently pick up content if that variable is later defined for an unrelated purpose. Drop the line, or define it explicitly in `Makefile.leveldb.include` if SSE4.2-accelerated leveldb is actually intended.
In `src/bitcoin-chainstate.cpp`:
- [SUGGESTION] lines 307-327: Shutdown does not explicitly tear down Dash kernel objects before UnsetGlobals
The epilogue stops the scheduler, flushes chainstates, unregisters the background scheduler, and then calls `init::UnsetGlobals()` — but the Dash-specific kernel objects (`chain_helper`, `llmq_ctx`, `dmnman`, `evodb`) are left to be destroyed implicitly when `main()` returns, which happens *after* `init::UnsetGlobals()` runs. The normal node shutdown at `src/init.cpp:405` calls `DashChainstateSetupClose(chain_helper, dmnman, llmq_ctx, ...)` and `evodb.reset()` *before* final global teardown. The file's own comment at line 308 states "Without this precise shutdown sequence, there will be a lot of nullptr dereferencing and UB." Even though the binary is experimental, mirroring the node's destruction order would avoid an exit-time crash class as the kernel surface grows.
| # Required for obj/build.h to be generated first. | ||
| # More details: https://www.gnu.org/software/automake/manual/html_node/Built-Sources-Example.html | ||
| bitcoin_chainstate-clientversion.$(OBJEXT): obj/build.h | ||
| # |
There was a problem hiding this comment.
🟡 Suggestion: obj/build.h dependency rule uses old upstream target name
Because dash_chainstate sets per-target CPPFLAGS/CXXFLAGS (lines 1306-1307), automake compiles its sources to per-target objects named dash_chainstate-<basename>.$(OBJEXT) (same convention as libbitcoin_util_a-clientversion.$(OBJEXT) on line 482). The rule bitcoin_chainstate-clientversion.$(OBJEXT): obj/build.h was copied verbatim from upstream and references a file automake will never produce for this binary, so it never enforces that obj/build.h is generated before clientversion.o is compiled. The PR's own merge note says the binary was renamed but "Names of source files and some variables are untouched" — this rule is one of the spots that did need to be renamed for the renamed target.
💡 Suggested change
| # Required for obj/build.h to be generated first. | |
| # More details: https://www.gnu.org/software/automake/manual/html_node/Built-Sources-Example.html | |
| bitcoin_chainstate-clientversion.$(OBJEXT): obj/build.h | |
| # | |
| # Required for obj/build.h to be generated first. | |
| # More details: https://www.gnu.org/software/automake/manual/html_node/Built-Sources-Example.html | |
| dash_chainstate-clientversion.$(OBJEXT): obj/build.h | |
| # |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/Makefile.am`:
- [SUGGESTION] lines 1318-1321: obj/build.h dependency rule uses old upstream target name
Because `dash_chainstate` sets per-target CPPFLAGS/CXXFLAGS (lines 1306-1307), automake compiles its sources to per-target objects named `dash_chainstate-<basename>.$(OBJEXT)` (same convention as `libbitcoin_util_a-clientversion.$(OBJEXT)` on line 482). The rule `bitcoin_chainstate-clientversion.$(OBJEXT): obj/build.h` was copied verbatim from upstream and references a file automake will never produce for this binary, so it never enforces that `obj/build.h` is generated before `clientversion.o` is compiled. The PR's own merge note says the binary was renamed but "Names of source files and some variables are untouched" — this rule is one of the spots that did need to be renamed for the renamed target.
| dash_chainstate_LDADD = \ | ||
| $(LIBBITCOIN_CRYPTO) \ | ||
| $(LIBUNIVALUE) \ | ||
| $(LIBSECP256K1) \ | ||
| $(LIBDASHBLS) \ | ||
| $(LIBLEVELDB) \ | ||
| $(LIBLEVELDB_SSE42) \ | ||
| $(LIBMEMENV) |
There was a problem hiding this comment.
🟡 Suggestion: dash_chainstate_LDADD references undefined variable LIBLEVELDB_SSE42
LIBLEVELDB_SSE42 is not defined anywhere in the tree. src/Makefile.leveldb.include only defines LIBLEVELDB = $(LIBLEVELDB_INT) $(LIBCRC32C) and LIBMEMENV; a grep for LIBLEVELDB_SSE42 finds only this LDADD line. Make silently expands an unset variable to empty so this is dead text rather than a build failure today, but it is misleading (implies SSE4.2 acceleration is being linked when it isn't) and would silently pick up content if that variable is later defined for an unrelated purpose. Drop the line, or define it explicitly in Makefile.leveldb.include if SSE4.2-accelerated leveldb is actually intended.
💡 Suggested change
| dash_chainstate_LDADD = \ | |
| $(LIBBITCOIN_CRYPTO) \ | |
| $(LIBUNIVALUE) \ | |
| $(LIBSECP256K1) \ | |
| $(LIBDASHBLS) \ | |
| $(LIBLEVELDB) \ | |
| $(LIBLEVELDB_SSE42) \ | |
| $(LIBMEMENV) | |
| dash_chainstate_LDADD = \ | |
| $(LIBBITCOIN_CRYPTO) \ | |
| $(LIBUNIVALUE) \ | |
| $(LIBSECP256K1) \ | |
| $(LIBDASHBLS) \ | |
| $(LIBLEVELDB) \ | |
| $(LIBMEMENV) |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/Makefile.am`:
- [SUGGESTION] lines 1309-1316: dash_chainstate_LDADD references undefined variable LIBLEVELDB_SSE42
`LIBLEVELDB_SSE42` is not defined anywhere in the tree. `src/Makefile.leveldb.include` only defines `LIBLEVELDB = $(LIBLEVELDB_INT) $(LIBCRC32C)` and `LIBMEMENV`; a grep for `LIBLEVELDB_SSE42` finds only this LDADD line. Make silently expands an unset variable to empty so this is dead text rather than a build failure today, but it is misleading (implies SSE4.2 acceleration is being linked when it isn't) and would silently pick up content if that variable is later defined for an unrelated purpose. Drop the line, or define it explicitly in `Makefile.leveldb.include` if SSE4.2-accelerated leveldb is actually intended.
| epilogue: | ||
| // Without this precise shutdown sequence, there will be a lot of nullptr | ||
| // dereferencing and UB. | ||
| scheduler.stop(); | ||
| if (chainman.m_load_block.joinable()) chainman.m_load_block.join(); | ||
| StopScriptCheckWorkerThreads(); | ||
|
|
||
| GetMainSignals().FlushBackgroundCallbacks(); | ||
| { | ||
| LOCK(cs_main); | ||
| for (CChainState* chainstate : chainman.GetAll()) { | ||
| if (chainstate->CanFlushToDisk()) { | ||
| chainstate->ForceFlushStateToDisk(); | ||
| chainstate->ResetCoinsViews(); | ||
| } | ||
| } | ||
| } | ||
| GetMainSignals().UnregisterBackgroundSignalScheduler(); | ||
|
|
||
| init::UnsetGlobals(); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Shutdown does not explicitly tear down Dash kernel objects before UnsetGlobals
The epilogue stops the scheduler, flushes chainstates, unregisters the background scheduler, and then calls init::UnsetGlobals() — but the Dash-specific kernel objects (chain_helper, llmq_ctx, dmnman, evodb) are left to be destroyed implicitly when main() returns, which happens after init::UnsetGlobals() runs. The normal node shutdown at src/init.cpp:405 calls DashChainstateSetupClose(chain_helper, dmnman, llmq_ctx, ...) and evodb.reset() before final global teardown. The file's own comment at line 308 states "Without this precise shutdown sequence, there will be a lot of nullptr dereferencing and UB." Even though the binary is experimental, mirroring the node's destruction order would avoid an exit-time crash class as the kernel surface grows.
💡 Suggested change
| epilogue: | |
| // Without this precise shutdown sequence, there will be a lot of nullptr | |
| // dereferencing and UB. | |
| scheduler.stop(); | |
| if (chainman.m_load_block.joinable()) chainman.m_load_block.join(); | |
| StopScriptCheckWorkerThreads(); | |
| GetMainSignals().FlushBackgroundCallbacks(); | |
| { | |
| LOCK(cs_main); | |
| for (CChainState* chainstate : chainman.GetAll()) { | |
| if (chainstate->CanFlushToDisk()) { | |
| chainstate->ForceFlushStateToDisk(); | |
| chainstate->ResetCoinsViews(); | |
| } | |
| } | |
| } | |
| GetMainSignals().UnregisterBackgroundSignalScheduler(); | |
| init::UnsetGlobals(); | |
| } | |
| GetMainSignals().FlushBackgroundCallbacks(); | |
| { | |
| LOCK(cs_main); | |
| for (CChainState* chainstate : chainman.GetAll()) { | |
| if (chainstate->CanFlushToDisk()) { | |
| chainstate->ForceFlushStateToDisk(); | |
| chainstate->ResetCoinsViews(); | |
| } | |
| } | |
| } | |
| node::DashChainstateSetupClose(chain_helper, dmnman, llmq_ctx, /*mempool=*/nullptr); | |
| evodb.reset(); | |
| GetMainSignals().UnregisterBackgroundSignalScheduler(); | |
| init::UnsetGlobals(); | |
| } |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/bitcoin-chainstate.cpp`:
- [SUGGESTION] lines 307-327: Shutdown does not explicitly tear down Dash kernel objects before UnsetGlobals
The epilogue stops the scheduler, flushes chainstates, unregisters the background scheduler, and then calls `init::UnsetGlobals()` — but the Dash-specific kernel objects (`chain_helper`, `llmq_ctx`, `dmnman`, `evodb`) are left to be destroyed implicitly when `main()` returns, which happens *after* `init::UnsetGlobals()` runs. The normal node shutdown at `src/init.cpp:405` calls `DashChainstateSetupClose(chain_helper, dmnman, llmq_ctx, ...)` and `evodb.reset()` *before* final global teardown. The file's own comment at line 308 states "Without this precise shutdown sequence, there will be a lot of nullptr dereferencing and UB." Even though the binary is experimental, mirroring the node's destruction order would avoid an exit-time crash class as the kernel surface grows.
Issue being fixed or feature implemented
This PR is a first backport of the
libdashkernelproject, see for details: bitcoin#24303What was done?
This PR introduces an example/demo dash-chainstate executable using said library which can print out information about a datadir and take in new blocks on stdin.
There are multiple PR that has been extracted from this PR and merged independently:
refactor: remove dependencies of CChainState on CMasternodeSync #7212Beside these PR multiple other PR has been done in the past, such as: #7178, #7070, #7008, #6992, #6959, #7106 (by Udjin), #7065 (by kwvg) and many other PRs.
This PR:
bitcoin-chainstatebitcoin/bitcoin#24304There are several important notes about implementation:
::UnloadBlockIndextoBlockManagerbitcoin/bitcoin#22564, not backportedThere are several modules that should be removed in the near future from dash-chainstate binary, but out-of-scope of this PR:
How Has This Been Tested?
New binary
dash-chainstateis produced, functional & unit tests succeed.Run dash-chainstate:
Breaking Changes
N/A
Checklist: